Rework handling of panning to identified features - #4591
Conversation
We will use it to jump to the highlighted feature once we know the available area of the map that is not covered by the panel
- Use the renamed previewPanelHeight property instead of signal params
- Move all highlighting and form opening to a identifyFeature()
- Don't pan if whole feature is near visible map center - Center feature to visible map if it fits - Center clicked location to visible map if whole does not fit
- Add method to calculate the map extent required for a geometry to fit the visible part of the map canvas when covered by a drawer - Use animated zoom when identifying features from a list and not on map
|
might help resolve #3845 as well |
Withalion
left a comment
There was a problem hiding this comment.
Nice! It would be great to rebase on current master as well since some time passed.
| // Holds the map coordinates of the point the user identified. NaN if identify was triggered from list of features | ||
| property point identifyLocation: Qt.point(NaN, NaN) | ||
|
|
||
| signal featureIdentified( var pair, var point ) |
There was a problem hiding this comment.
| signal featureIdentified( var pair, var point ) | |
| signal featureIdentified( FeatureLayerPair pair, qgsPoint clickedPoint ) |
It's about time we expose FeatureLayerPair & QgsPoint to QML properly
There was a problem hiding this comment.
Well yes, but do you think this PR is the proper place to do it?
| } | ||
| } | ||
|
|
||
| void TestUtilsFunctions::testWhereToPanWhenIdentifying() |
There was a problem hiding this comment.
Could we check here that the scale has not changed as well
There was a problem hiding this comment.
InputUtils::whereToPanWhenIdentifying() does not perform the panning, it only finds out the point that needs to be panned to.
It does not modify the settings at all.
Maybe better const' the InputMapSettings *` param to clarify the intent?
| } | ||
| } | ||
|
|
||
| void TestUtilsFunctions::testDrawerCompensatedExtent() |
There was a problem hiding this comment.
As well here I would have case for:
- when the map is just panned
- when feature is bigger then current extent, so the map zooms out
- when feature is smaller then current extent
There was a problem hiding this comment.
This only tests the extent calculation when compensating for the drawer height.
Testing the actual panning to and zooming to would be a completely different case.
|
@uclaros can you please imrpove the description of this issue. What's happening, what's the issue. Mostly for testers to understand the issue better. |
📦 Build Artifacts Ready
|
Plan A
This PR is a rework on how we jump to geometries. It was initially just a way to mitigate a GEOS crash when identifying self-overlapping multipart geometries, but quickly evolved!
Plan B
If this is too much for a last minute change, we can avoid hitting the geos bug by slightly modifying the current approach: Perform bounding box intersections instead of geometry intersections.
This will change the existing behavior, eg identifying C shaped geometry that is partially visible will not recenter the map canvas to the center of the visible part of the geometry, but to the visible part of the geom bbox.
Testing notes: